Skip to content

Support value-of<BackedEnum> #1082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Mar 23, 2022
Merged

Support value-of<BackedEnum> #1082

merged 18 commits into from
Mar 23, 2022

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Mar 18, 2022

@staabm
Copy link
Contributor Author

staabm commented Mar 18, 2022 •
edited
Loading

this part of the initial issue

Additionally PHPStan does not error on types that have this as array key: array<value-of, bool>, which means that it silently accepts incorrect code in this case (key-of has the same issue in this case, I guess that should just not be allowed though).
Playground example: phpstan.org/r/50d372e7-f7bf-49a2-8456-2eb89d97b7bc

sounds like a missing rule-check somewhere... I think this can be fixed in a separate PR (which I can work on after this)?

@staabm
Copy link
Contributor Author

staabm commented Mar 18, 2022 •
edited
Loading

(sorry for trial and error.. have no php 8.1 setup yet locally)

build error is unrelated

@staabm staabm mentioned this pull request Mar 18, 2022
Merged
ondrejmirtes
ondrejmirtes requested changes Mar 18, 2022
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a case to NodeScopeResolverTest that tests what value-of resolves to inside a method body.

staabm
staabm commented Mar 18, 2022
View reviewed changes
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 18, 2022

About this:

Additionally PHPStan does not error on types that have this as array key: array<value-of, bool>, which means that it silently accepts incorrect code in this case

Yeah, let's discuss this separately in another PR. The issue here is that value-of<something wrong> produces an ErrorType in TypeNodeResolver, and array<*ERROR*, X> gets cast to array<int|string, X> in TypeNodeResolver but it needs to stay array<*ERROR*, X> on this line:

phpstan-src/src/PhpDoc/TypeNodeResolver.php

Line 460 in 14cb258

$arrayType = new ArrayType(!$keyType instanceof NeverType ? ArrayType::castToArrayKeyType($keyType) : $keyType, $genericTypes[1]);

@staabm staabm force-pushed the value-of-enum branch 2 times, most recently from 1ca0150 to 90851b1 Compare March 18, 2022 17:09
@ondrejmirtes ondrejmirtes force-pushed the 1.5.x branch 3 times, most recently from ddd20b4 to 95d480b Compare March 18, 2022 19:53
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 21, 2022

Please add NodeScopeResolverTest case to verify it works as expected.

@staabm
Copy link
Contributor Author

staabm commented Mar 21, 2022

Please add NodeScopeResolverTest case to verify it works as expected.

done

ondrejmirtes
ondrejmirtes requested changes Mar 22, 2022
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously the implementation in TypeNodeResolver needs to change. Please iterate over ClassReflection::getEnumCases() to get to actual scalar values.

tests/PHPStan/Analyser/NodeScopeResolverTest.php Outdated
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Require is not needed here, make sure to run composer dump locally first.

Copy link
Contributor Author

@staabm staabm Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it locally.. the tests do not work without this require line.

even if I run composer dump beforehand

Copy link
Contributor Author

@staabm staabm Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh.. I need to /my/php8.1 composer dump and then it works

tests/PHPStan/Analyser/data/value-of-enum.php Outdated
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't good enough, right? Should be 'The Netherlands'|'United States' really.

Copy link
Member

@ondrejmirtes ondrejmirtes Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also why that bug isn't report in CallMethodsRuleTest.

Copy link
Contributor Author

@staabm staabm Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it. good catch.

staabm
staabm commented Mar 23, 2022
View reviewed changes
tests/PHPStan/Rules/Methods/data/call-method-in-enum.php
function doFooArray() {
$this->hello(CountryNo::NL);

// 'abc' does not match value-of<Country>
Copy link
Contributor Author

@staabm staabm Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as expected, we now see errors here.

@ondrejmirtes ondrejmirtes merged commit 7171941 into phpstan:1.5.x Mar 23, 2022
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 23, 2022

Thank you.

@staabm staabm deleted the value-of-enum branch March 23, 2022 16:11
This was referenced Mar 25, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

value-of<BackedEnum> would be cool to have

3 participants

@staabm @ondrejmirtes @clxmstaab